multi-ingress: cross-IdP SSO#5212
Conversation
zebot
added
the
ok-to-test
supersven
force-pushed
the
sventennie/multi-ingress-sso_cross_idp
branch
3 times, most recently
from
70c63ab to
63c5b7d
Compare
supersven
force-pushed
the
sventennie/multi-ingress-sso_cross_idp
branch
3 times, most recently
from
c70220c to
e7f3bf2
Compare
supersven
changed the title
supersven
force-pushed
the
sventennie/multi-ingress-sso_cross_idp
branch
from
e7f3bf2 to
d27326c
Compare
There was a problem hiding this comment.
Pull request overview
Adds support for multi-ingress “cross-IdP” SSO logins in Spar by falling back to a team-wide lookup (by email NameID) when the primary (issuer, NameID) lookup fails, and updates /sso/get-by-email discovery to work for any SSO user (not only SCIM-managed).
Changes:
- Extend Spar SAML finalize-login handling to support cross-IdP migration in multi-ingress mode (and add a dedicated config error for non-email NameIDs).
- Relax
/sso/get-by-emaillookup condition from “SCIM+SSO” to “SSO user”. - Add integration and unit tests plus documentation/changelog updates covering the new behavior.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| services/spar/test-integration/Test/Spar/AppSpec.hs | Updates integration test helper to call updated verdictHandler signature with SAML config. |
| services/spar/src/Spar/Error.hs | Adds and renders a new error for invalid multi-ingress cross-IdP configuration (non-email NameID). |
| services/spar/src/Spar/App.hs | Implements multi-ingress cross-IdP fallback flow + structured logging changes. |
| services/spar/src/Spar/API.hs | Plumbs SAML config/host info into finalize-login handling and updates logger constraints. |
| libs/wire-subsystems/test/unit/Wire/IdPSubsystem/InterpreterSpec.hs | Adjusts unit test expectation to “non SSO user” behavior for get-by-email. |
| libs/wire-subsystems/src/Wire/IdPSubsystem/Interpreter.hs | Changes /sso/get-by-email eligibility check from SCIM+SSO to SSO. |
| integration/test/Test/Spar/MultiIngressCrossIdpSso.hs | Adds new integration test suite for cross-IdP migration and related error cases. |
| integration/integration.cabal | Registers new integration test module. |
| docs/src/developer/reference/config-options.md | Documents cross-IdP fallback algorithm and updates get-by-email criteria. |
| changelog.d/2-features/multi-ingress-cross-IdP-SSO | Adds changelog entry describing cross-IdP SSO behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| -- This used to check if the user is SCIM AND SSO! The RFC ("2025-05-12 | ||
| -- RFC: Default SSO flow for team by host domain") is not really | ||
| -- unambiguous about this. The customer currently provisions non-SCIM. | ||
| isSsoUser :: User -> Bool | ||
| isSsoUser = isJust . userSSOId |
There was a problem hiding this comment.
This might be subject to change in case the customer decides to go for SCIM. Then, we could make this condition stricter (requiring SSO AND SCIM).
fisx
left a comment
fisx
left a comment
There was a problem hiding this comment.
I read the docs and have some questions, more feedback coming up!
| 1. **NameID must be an email address.** Username-based `NameID`s are rejected | ||
| to avoid ambiguity across authenticating IdPs. |
There was a problem hiding this comment.
[nit] admins need to manually guarantee that email addresses are unique, so why can't they do the same thing here?
There was a problem hiding this comment.
@emil-wire and I were uncertain about this as well. I made the hand-wavy decision, because in my experience with other web services, my email address is much less likely to conflict than my desired username when I register somewhere.
However, the customer is not much into emails as fetching those may reveal relationships as well AND @julialongtin promised that all data will be located in one database (used by all IdPs)
So, this could actually be subject to change. 🤔
| assertion's issuer and the incoming `Z-Host` header (exact match). If no exact | ||
| match is found and the team has exactly one IdP configuration, that one is used | ||
| unconditionally (no issuer or domain check). If neither condition is met, the |
There was a problem hiding this comment.
Why pick a non-matching IdP, and not just reject right away?
There was a problem hiding this comment.
When I implemented this "highlander principle" some time ago, I wanted to cover the case where one IdP could be configured to be usable by multiple domains.
However - as we now got more experience / intuition / insights about the topic at hand - this the single IdP / multiple ingresses approach would reveal a common relationship (all belong to one IdP). This contradicts the intention of multi-ingress. So, I'm leaning to change this.
Also, @JuhiMehta24 got pretty confused by this during testing.
There was a problem hiding this comment.
supersven
force-pushed
the
sventennie/multi-ingress-sso_cross_idp
branch
from
efc0e2a to
a149340
Compare
Allow users to login with all team IdPs. This is required because there's usually one IdP per ingress in a team and users should be able to login on all ingresses.
supersven
force-pushed
the
sventennie/multi-ingress-sso_cross_idp
branch
from
a149340 to
4cce453
Compare
I've covered this usecase in the documentation. To avoid duplication, please look at the docs in this PR.
Ticket: https://wearezeta.atlassian.net/browse/WPB-25161
Security notes:
/sso/get-by-emailfrom SCIM SSO to SSO userFurther reading:
Checklist
changelog.d